Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Apply restoring to all tracers #214

Merged
merged 4 commits into from
Jan 16, 2018
Merged

Apply restoring to all tracers #214

merged 4 commits into from
Jan 16, 2018

Conversation

mnlevy1981
Copy link
Collaborator

Apply restoring to all tracers, not just PO4, NO3, and SiO3. Computes
tendency on its own and then applies restoring, so there is an
order-of-operations change on these tendencies (and the ALK tendency
depends on the NO3 tendency, so it now gets the pre-restoring tendency).

Apply restoring to all tracers, not just PO4, NO3, and SiO3. Computes
tendency on its own and then applies restoring, so there is an
order-of-operations change on these tendencies (and the ALK tendency
depends on the NO3 tendency, so it now gets the pre-restoring tendency).
@mnlevy1981
Copy link
Collaborator Author

mnlevy1981 commented Jan 8, 2018

For testing, I ran a POP T62_g37 case for three days and compared to a baseline. I did this in three steps:

  1. On master, I verified that removing ALK from tracer_restore_vars only changed the ALK_RESTORE_TEND field (as pointed out in tracer restoring tendency is only added to tracer tendencies for NO3, SiO3, PO4 #213 we do not apply the restoring tendency to ALK)

  2. Then I applied the changes in this pull request but still left 'ALK' out of 'tracer_restore_vars'. I saw small changes in PO4, NO3, and SiO3 but a larger difference in ALK (which I attribute to seeing a different NO3 tendency):

    Variable: PO4 ...
    ... variable is double ...
    ... Biggest (absolute) diff = 2.69784194983913e-14
    ... Biggest (relative) diff = 3.399820284831256e-15
    
    Variable: NO3 ...
    ... variable is double ...
    ... Biggest (absolute) diff = 2.869116055848053e-10
    ... Biggest (relative) diff = 5.955086504735507e-12
    
    Variable: SiO3 ...
    ... variable is double ...
    ... Biggest (absolute) diff = 7.149836278586008e-14
    ... Biggest (relative) diff = 3.26385282545721e-16
    
    Variable: ALK ...
    ... variable is double ...
    ... Biggest (absolute) diff = 0.03164020566555337
    ... Biggest (relative) diff = 6.834567472502646e-06
    
  3. Then I turned ALK restoring back on, to verify that this PR does apply the restoring:

    Variable: PO4 ...
    ... variable is double ...
    ... Biggest (absolute) diff = 6.961764498214507e-12
    ... Biggest (relative) diff = 8.773215258462089e-13
    
    Variable: NO3 ...
    ... variable is double ...
    ... Biggest (absolute) diff = 1.086510242886973e-07
    ... Biggest (relative) diff = 2.255141429878699e-09
    
    Variable: SiO3 ...
    ... variable is double ...
    ... Biggest (absolute) diff = 2.843769664195861e-11
    ... Biggest (relative) diff = 1.298161984664459e-13
    
    Variable: ALK ...
    ... variable is double ...
    ... Biggest (absolute) diff = 3.556573223929718
    ... Biggest (relative) diff = 0.0007682516329629172
    

@mnlevy1981
Copy link
Collaborator Author

One question that has come up is when to add restoring to the tendencies. The ALK tendency, for example, is

dtracers(alk_ind) = -dtracers(no3_ind) + dtracers(nh4_ind) + c2 * P_CaCO3_remin

do auto_ind = 1, auto_cnt
   if (marbl_tracer_indices%auto_inds(auto_ind)%CaCO3_ind > 0) then
      dtracers(alk_ind) = dtracers(alk_ind) &
           + c2 * (f_graze_CaCO3_REMIN * auto_graze(auto_ind) * QCaCO3(auto_ind) - CaCO3_form(auto_ind))
   end if
end do

Should the dtracers(no3_ind) and dtracers(nh4_ind) terms include NO3 and NH4 restoring, respectively? (The default POP setup restores NO3 but not NH4). The current implementation in this PR does not include restoring, but there are a couple different ways to change the code so it does if that's a better way to do it.

This update makes sure interior restoring is applied to ALL tracers; it
is done in a manner such that restoring is applied after the budget
computations, so those no longer need to be adjusted to subtract out the
restoring term.

Also cleaned up one change from previous code, reverting back to
convention of putting mathematical operator after the continuation
character when wrapping lines.
Inadvertently added whitespace at top of document, removed from bottom
of document.
@mnlevy1981
Copy link
Collaborator Author

As expected, 6b59dca only had round-off level effect on a few of the budget terms:

Variable: Jint_Ntot ...
... Biggest (absolute) diff = 1.539567084929416e-19

Variable: Jint_100m_Ntot ...
... Biggest (relative) diff = 4.15157868043084e-17

Variable: Jint_Ptot ...
... Biggest (absolute) diff = 1.572093150103981e-20

Variable: Jint_100m_Ptot ...
... Biggest (relative) diff = 2.82969226809634e-17

Variable: Jint_Sitot ...
... Biggest (absolute) diff = 8.483881999699072e-20

Variable: Jint_100m_Sitot ...
... Biggest (relative) diff = 3.374337348949718e-18

This came from my one-off tests, will re-run POP tests to make sure nothing breaks unexpectedly and then bring these changes to master.

@mnlevy1981 mnlevy1981 merged commit ea6dbff into marbl-ecosys:master Jan 16, 2018
@mnlevy1981 mnlevy1981 deleted the bugfix/apply_restoring branch January 16, 2018 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant